✨ Implement controlled unitary embedding#1822
Conversation
simon1hofmann
added
c++
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…t indices and improve matrix embedding logic. Add unit test for sparse wire indices to ensure correctness.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates QCO matrix wire/bit conventions, adds ChangesRegister-aware controlled unitary embedding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp (1)
147-155: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAssert the optional matrix before dereferencing it.
getUnitaryMatrix()can returnstd::nullopt; this test should fail cleanly instead of dereferencing a disengaged optional if resolution regresses.Proposed fix
auto ctrlOp = *funcOp.getBody().getOps<CtrlOp>().begin(); auto matrix = ctrlOp.getUnitaryMatrix(); + ASSERT_TRUE(matrix); const Matrix4x4 expected =🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp` around lines 147 - 155, The test around getUnitaryMatrix in test_qco_ir_matrix.cpp should not dereference the optional directly, since ctrlOp.getUnitaryMatrix() may return std::nullopt. Update the assertion flow to first verify the optional is engaged before using matrix->isApprox(expected), so the test fails cleanly if unitary matrix resolution regresses. Use the getUnitaryMatrix result variable and the existing expectedMatrixFromComputation setup to keep the check in the same test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Utils/Matrix.h`:
- Around line 745-747: Update the documentation for the matrix utility that
takes control and target qubits so the parameter comments clearly say these are
embedding-space/local wire indices used after remapping, not raw
program-register indices. Adjust the wording in the comment attached to the
relevant matrix application helper so callers like CtrlOp understand they must
pass the compact positions corresponding to numQubits/participating rather than
sparse global wire numbers.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp`:
- Around line 48-49: Drop the top-level const from MLIR handle-type parameters
in the QCO modifiers API so the signatures match MLIR conventions. Update
programQubitIndex and any related declarations/definitions in CtrlOp.cpp (and
the corresponding header or callers if needed) to take Value and ValueRange
without const, while keeping the existing logic unchanged.
- Around line 386-391: The duplicate-removal logic in the participating qubit
list is wrong because `std::ranges::unique(participating).end()` returns the
original end iterator, so the erase does nothing and duplicates can still reach
`embedControlledUnitary`. Update the deduplication in `CtrlOp::` the block that
builds `participating` to erase the returned unique subrange itself, using the
result of `std::ranges::unique(participating)` rather than its `.end()` member,
so repeated wire indices are actually removed before validation.
In `@mlir/lib/Dialect/QCO/Utils/Matrix.cpp`:
- Around line 218-227: The validation in Matrix::embedUnitary currently only
checks wire ranges, so add explicit duplicate/overlap checks before building the
embedding. In the same section that validates controlQubits and targetQubits,
reject any repeated entries within targetQubits and any wire that appears in
both controlQubits and targetQubits, so the later mask construction cannot
silently collapse distinct operands. Keep the existing range asserts, but
tighten the preconditions in embedUnitary to fail fast on invalid control/target
wire sets.
---
Outside diff comments:
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp`:
- Around line 147-155: The test around getUnitaryMatrix in
test_qco_ir_matrix.cpp should not dereference the optional directly, since
ctrlOp.getUnitaryMatrix() may return std::nullopt. Update the assertion flow to
first verify the optional is engaged before using matrix->isApprox(expected), so
the test fails cleanly if unitary matrix resolution regresses. Use the
getUnitaryMatrix result variable and the existing expectedMatrixFromComputation
setup to keep the check in the same test case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c9588ee-b7fa-4990-a30d-6395baca21b2
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QCO/Utils/Matrix.hmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/Utils/Matrix.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp (2)
47-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Doxygen comments for the new internal helpers.
These new C++ helpers have no header declarations where API docs can live, so add brief
///comments here.Proposed documentation
+/// Returns the statically-known program qubit index for `@p` qubit, if available. [[nodiscard]] static std::optional<std::size_t> programQubitIndex(Value qubit) { @@ +/// Resolves all `@p` qubits to statically-known program qubit indices. [[nodiscard]] static std::optional<SmallVector<std::size_t>> resolveQubitIndices(ValueRange qubits) {As per coding guidelines, C++ code “MUST use Doxygen-style comments in C++ code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp` around lines 47 - 81, The new internal helpers lack Doxygen-style documentation in the implementation file, so add brief `///` comments directly above `programQubitIndex` and `resolveQubitIndices` in `CtrlOp.cpp`. Describe each helper’s purpose, inputs, and return behavior at a high level so the C++ code follows the project’s Doxygen comment requirement.Source: Coding guidelines
364-396: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap the total participating wires before embedding.
Line 358 only limits controls, but Line 394 now builds a matrix over
participating.size(). A case like 31 controls plus one target bypasses the guard and asksembedControlledUnitaryto allocate a 32-wire embedding.Proposed fix
const auto controlQubits = resolveQubitIndices(getInputControls()); const auto targetQubits = resolveQubitIndices(getInputTargets()); if (!controlQubits || !targetQubits) { return std::nullopt; } + SmallVector<std::size_t> participating; + participating.append(*controlQubits); + participating.append(*targetQubits); + llvm::sort(participating); + participating.erase(llvm::unique(participating), participating.end()); + if (participating.size() >= 32) { + llvm::reportFatalUsageError( + "Creating the unitary matrix for a CtrlOp with more than 31 " + "participating qubits is not supported due to memory constraints."); + } + // Inner unitary on targets: one body op or a composed single-qubit sequence. std::optional<DynamicMatrix> targetMatrix; @@ - SmallVector<std::size_t> participating; - participating.append(*controlQubits); - participating.append(*targetQubits); - llvm::sort(participating); - participating.erase(llvm::unique(participating), participating.end()); - const auto toLocal = [&](const std::size_t wire) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp` around lines 364 - 396, The controlled-unitary embedding in CtrlOp::getUnitaryMatrix now uses participating.size(), so the total number of control plus target wires must be capped before calling embedControlledUnitary. Update the existing size guard near getInputControls/getInputTargets to reject cases where the combined participating wires would exceed the allowed maximum, not just when controlQubits is too large, and keep the check aligned with the local embedding logic that builds participating and maps toLocal.mlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp (1)
235-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full controlled matrix here.
This only checks 6 of the 16 entries, so a regression that introduces stray nonzeros or swaps an additional basis pair would still pass. Comparing against the full expected
4x4matrix would make this regression test much harder to satisfy accidentally.Suggested tightening
TEST(DynamicMatrix, EmbedControlledUnitary) { // Controlled-X with control on wire 0 and target on wire 1. const auto controlledX = embedControlledUnitary(2, {0}, {1}, DynamicMatrix(pauliX())); - EXPECT_EQ(controlledX(0, 0), 1.0); - EXPECT_EQ(controlledX(1, 1), 0.0); - EXPECT_EQ(controlledX(1, 3), 1.0); - EXPECT_EQ(controlledX(2, 2), 1.0); - EXPECT_EQ(controlledX(3, 1), 1.0); - EXPECT_EQ(controlledX(3, 3), 0.0); + const auto expected = Matrix4x4::fromElements(1, 0, 0, 0, // + 0, 0, 0, 1, // + 0, 0, 1, 0, // + 0, 1, 0, 0); + EXPECT_TRUE(controlledX.isApprox(expected)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp` around lines 235 - 244, Tighten the DynamicMatrix.EmbedControlledUnitary test by asserting the entire controlledX matrix instead of only a few entries. In test_unitary_matrix.cpp within TEST(DynamicMatrix, EmbedControlledUnitary), replace the partial EXPECT_EQ checks on controlledX with a full 4x4 expectation against the correct controlled-X matrix so stray nonzeros or swapped basis pairs are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp`:
- Around line 47-81: The new internal helpers lack Doxygen-style documentation
in the implementation file, so add brief `///` comments directly above
`programQubitIndex` and `resolveQubitIndices` in `CtrlOp.cpp`. Describe each
helper’s purpose, inputs, and return behavior at a high level so the C++ code
follows the project’s Doxygen comment requirement.
- Around line 364-396: The controlled-unitary embedding in
CtrlOp::getUnitaryMatrix now uses participating.size(), so the total number of
control plus target wires must be capped before calling embedControlledUnitary.
Update the existing size guard near getInputControls/getInputTargets to reject
cases where the combined participating wires would exceed the allowed maximum,
not just when controlQubits is too large, and keep the check aligned with the
local embedding logic that builds participating and maps toLocal.
In `@mlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp`:
- Around line 235-244: Tighten the DynamicMatrix.EmbedControlledUnitary test by
asserting the entire controlledX matrix instead of only a few entries. In
test_unitary_matrix.cpp within TEST(DynamicMatrix, EmbedControlledUnitary),
replace the partial EXPECT_EQ checks on controlledX with a full 4x4 expectation
against the correct controlled-X matrix so stray nonzeros or swapped basis pairs
are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 596decd9-a052-4758-872e-ca576816162e
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QCO/Utils/Matrix.hmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/Utils/Matrix.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp
|
Just gave this some thought as discussed offline today. As a result, I believe the other direction we discussed is a little more promising. That is, the methods of the individual operations just report a matrix representation in a canonical order. That's order is the one where the the control qubits are the MSQ's and the target qubits are the LSQ's, where we typically use big endian notation to write down bitstrings (q_n-1 ... q0). The DD package always considers global/cirxuit-wide qubit indices; so it makes a distinction between cx(0,1) and cx(1,0) when constructing the DDs for these operations. In mqt-cc, the matrix would always be the cx(1,0) one. This also fits well with how Qiskit handles this: https://github.com/Qiskit/qiskit/blob/9ff63334c989c8c6e0ddd303a224ddbd907f0b6e/crates/circuit/src/gate_matrix.rs#L87 |
I also believe that is the way to go, closing this PR and the related Issue #1821. |
2 participants
Description
Fixes incorrect controlled-gate unitary matrices in QCO and unifies matrix wire indexing on the
QuantumComputationconvention.Problem:
CtrlOp::getUnitaryMatrix()embedded the inner unitary viaDynamicMatrix::setBottomRightCorner (I_{2^c} ⊗ U), which is only correct when controls sit on the most significant wires. For general wire placement (e.g.cx(q[0], q[1])), the result did not matchQuantumComputation.Solution:
embedControlledUnitary()to place a multi-controlled unitary using actual control/target register indices (QC little-endian: qubiti= biti).qco.staticand constantqtensor.extractoperands.embedInNqubit,kron, and related helpers on the same bit ordering (replacing the old MSB-first embedding).DynamicMatrix::setBottomRightCornerAPI.Fixes #1821
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.